@acul71: Can you please take a look why these checks are failing with my changes, I am not understanding.

Added the certhash protcol, along with the test-suite

Added webrtc and webrtc-direct in 4507543, along with maddr test cases in test_multiaddr.py

@seetadev @acul71: Did some tweaks in aa2202d to resolve the CI checks, please take a look and see if anything else remains, other than make docs content addition in this PR.

Will add the doc content for all protocols in a separate PR.

@lla-dane

PR #97 Review: Add SNI, NOISE, CERTHASH, WEBRTC, WEBRTC-DIRECT in py-multiaddr

Summary

This PR adds support for 5 new multiaddr protocols to py-multiaddr:

• SNI (Server Name Indication)
• NOISE (Noise Protocol Framework)
• CERTHASH (Certificate Hash)
• WEBRTC (WebRTC)
• WEBRTC-DIRECT (WebRTC Direct)

The implementation follows the go-multiaddr reference implementation and includes comprehensive test coverage.

Detailed Review

✅ Protocol Codes Verification

All protocol codes match the go-multiaddr reference implementation:

✅ Protocol Configurations

Protocol configurations correctly match the go-multiaddr implementation:

✅ CERTHASH Implementation

The CERTHASH codec implementation correctly matches the go-multiaddr transcoder:

Go Implementation:

func certHashStB(s string) ([]byte, error) {
    _, data, err := multibase.Decode(s)
    if err != nil {
        return nil, err
    }
    if _, err := mh.Decode(data); err != nil {
        return nil, err
    }
    return data, nil
}

func certHashBtS(b []byte) (string, error) {
    return multibase.Encode(multibase.Base64url, b)
}

func validateCertHash(b []byte) error {
    _, err := mh.Decode(b)
    return err
}

Python Implementation:

def to_bytes(self, proto: Any, string: str) -> bytes:
    decoded_bytes = multibase.decode(string)
    self.validate(decoded_bytes)
    return decoded_bytes

def to_string(self, proto: Any, buf: bytes) -> str:
    self.validate(buf)
    encoded_string = multibase.encode("base64url", buf)
    return encoded_string.decode("utf-8")

def validate(self, b: bytes) -> None:
    multihash.decode(b)

Analysis:

• ✅ String-to-bytes conversion matches go implementation
• ✅ Bytes-to-string conversion matches go implementation
• ✅ Validation logic matches go implementation
• ✅ Uses base64url encoding as specified in go implementation

✅ Test Coverage

Comprehensive test coverage has been added:

Multiaddr String Tests

# SNI test case
"/ip4/127.0.0.1/tcp/443/tls/sni/example.com/http/http-path/foo"

# NOISE test case  
"/ip4/127.0.0.1/tcp/127/noise"

# CERTHASH test cases
"/ip4/127.0.0.1/udp/1234/quic-v1/webtransport/certhash/b2uaraocy6yrdblb4sfptaddgimjmmpy"
"/ip4/127.0.0.1/udp/1234/quic-v1/webtransport/certhash/b2uaraocy6yrdblb4sfptaddgimjmmpy/certhash/zQmbWTwYGcmdyK9CYfNBcfs9nhZs17a6FQ4Y8oea278xx41"

# WEBRTC test cases
"/ip4/127.0.0.1/tcp/127/webrtc-direct"
"/ip4/127.0.0.1/tcp/127/webrtc"

CERTHASH Codec Tests

• ✅ Valid roundtrip conversion
• ✅ Invalid multihash bytes handling
• ✅ Invalid multibase content handling
• ✅ Invalid multibase string handling
• ✅ Direct validation function testing

✅ Code Quality

• Documentation: CERTHASH codec includes comprehensive docstrings
• Error Handling: Proper exception handling with descriptive error messages
• Type Hints: Consistent use of type hints throughout
• Code Style: Follows existing codebase patterns

✅ Dependencies

The implementation correctly uses existing dependencies:

• multibase for base64url encoding/decoding
• multihash for multihash validation
• No new external dependencies introduced

Issues Found

✅ Fixed Issues

1. Multihash API Usage: The test was using multihash.encode() which is correct for the py-multihash library used in CI, but was initially using multihash.digest().encode() which is from a different library.

2. Newsfragment Issue Number: The newsfragment was initially named 97.feature.rst (PR number) but was correctly renamed to 181.feature.rst to match issue #181.

⚠️ Minor Issues

1. Test Function Naming: The test function test_certhash_memory_validate_function() has an incorrect name - it should be test_certhash_validate_function() since it's testing the certhash codec, not memory.

2. Missing Documentation: The new protocols could benefit from documentation in the main module docstring or examples.

Recommendations

✅ Approve with Minor Changes

This PR is ready for approval with the following minor suggestions:

1. Fix test function name: Rename test_certhash_memory_validate_function() to test_certhash_validate_function()

2. Consider adding documentation: Add brief documentation for the new protocols in the main module or examples.

Note: The multihash API usage issue has been fixed during review.

Conclusion

This PR successfully implements all 5 new multiaddr protocols with:

• ✅ Correct protocol codes matching go-multiaddr
• ✅ Proper protocol configurations
• ✅ Accurate CERTHASH implementation
• ✅ Comprehensive test coverage
• ✅ Good code quality

The implementation is faithful to the go-multiaddr reference implementation and maintains consistency with the existing py-multiaddr codebase.

Recommendation: APPROVE ✅

@lla-dane : Great contribution. Appreciate your efforts.

Wish if you could resolve the merge conflicts. Looking forward to arriving at a good conclusion on this important PR.

Will re-run the CI/CD pipeline after the merge conflicts are resolved.

Can you please open a discussion page with updated notes and screencast on testing the PR? We can take up the examples in another PR.

@acul71 : Thank you so much Luca for reviewing the PR. Appreciate your support.

Fixed the merge conflicts. @seetadev. Lets see the CI run.

@lla-dane : This is indeed, fantastic. CI/CD issues resolved. Please spend sometime on these minor issues observed earlier:

Test Function Naming: The test function test_certhash_memory_validate_function() has an incorrect name - it should be test_certhash_validate_function() since it's testing the certhash codec, not memory.

Missing Documentation: The new protocols could benefit from documentation in the main module docstring or examples. Please add summarized documentation here. We will follow it up in details on a discussion page.

The PR is almost ready for a final review + merge.